-
Notifications
You must be signed in to change notification settings - Fork 574
Feature view mode toggle #644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
WalkthroughAdds a view-mode feature: a new Redux slice for view mode, a toggle UI, a GalleryView component supporting chronological/grid/list/masonry layouts, store integration and Home page wiring; also updates backend/.gitignore to ignore environment artifacts. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ViewModeToggle
participant ReduxStore as "Redux Store"
participant GalleryView
User->>ViewModeToggle: click mode button
ViewModeToggle->>ReduxStore: dispatch(setViewMode(mode))
ReduxStore->>ReduxStore: update viewMode.mode
ReduxStore-->>GalleryView: notify state change
GalleryView->>GalleryView: re-render chosen layout (chronological/grid/list/masonry)
GalleryView-->>User: render images in selected view
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/.gitignore (1)
8-14: Consolidate environment-related entries with existing section.The added entries
venv/(line 11) and.env/(line 12) duplicate or overlap with existing ignore patterns at lines 111–117. While this doesn't break functionality, consolidating them would reduce maintenance burden and improve clarity.Consider removing lines 11–12 and adding
.sync-env/to the existing environment section (around line 117), or if.env/is intentionally distinct from.env, clarify the difference.Proposed fix: Move
.sync-env/to the environment section and remove duplicates.- - -.sync-env/ -venv/ -.env/ -Then add to the environment section (after line 117):
venv.bak/ +.sync-env/frontend/src/components/GalleryView.tsx (1)
180-180: Add fallback UI for unknown view modes.Returning
nullsilently when the mode doesn't match any known value provides no feedback. Consider rendering a fallback message or defaulting to chronological mode.Apply this diff to provide better user feedback:
- return null; + // Fallback to chronological if mode is somehow invalid + console.warn(`Unknown view mode: ${mode}, falling back to chronological`); + return ( + <ChronologicalGallery + images={images} + showTitle={true} + title={title} + onMonthOffsetsChange={onMonthOffsetsChange} + scrollContainerRef={scrollableRef} + /> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
backend/.gitignore(1 hunks)frontend/src/app/store.ts(2 hunks)frontend/src/components/GalleryView.tsx(1 hunks)frontend/src/components/ViewModeToggle.tsx(1 hunks)frontend/src/features/viewModeSlice.ts(1 hunks)frontend/src/pages/Home/Home.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/ViewModeToggle.tsx (2)
frontend/src/features/viewModeSlice.ts (1)
ViewMode(4-4)frontend/src/app/store.ts (1)
RootState(26-26)
frontend/src/pages/Home/Home.tsx (2)
frontend/src/components/ViewModeToggle.tsx (1)
ViewModeToggle(82-116)frontend/src/components/GalleryView.tsx (1)
GalleryView(64-181)
frontend/src/components/GalleryView.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/app/store.ts (1)
RootState(26-26)
🔇 Additional comments (7)
frontend/src/app/store.ts (1)
10-11: LGTM! Clean Redux integration.The viewMode slice is properly integrated into the store, following the same pattern as other reducers in the application.
Also applies to: 21-22
frontend/src/features/viewModeSlice.ts (1)
1-25: LGTM! Well-structured Redux slice.The slice is clean and follows Redux Toolkit best practices. The ViewMode type provides compile-time type safety for valid mode values.
frontend/src/pages/Home/Home.tsx (2)
19-20: LGTM! Component imports and rendering are properly integrated.The ViewModeToggle and GalleryView components are correctly imported and positioned in the layout.
Also applies to: 64-64
92-93: LGTM! Default export added.Adding a default export alongside the named export provides flexibility for imports.
frontend/src/components/ViewModeToggle.tsx (1)
62-118: LGTM! Clean toggle implementation with good accessibility.The component properly:
- Uses Redux hooks to read and update view mode state
- Includes
aria-pressedfor accessibility- Provides visual feedback for active state
- Uses lucide-react icons for clear visual representation
frontend/src/components/GalleryView.tsx (2)
35-44: LGTM! Robust path handling with fallback.The
toTauriUrlfunction correctly handles the case whereconvertFileSrcmight throw when not running in Tauri, falling back gracefully to the raw path.
46-62: LGTM! Clear image source resolution logic.The priority order (url → thumbnailPath → path → placeholder) is well-documented and handles various image source types appropriately.
| if (mode === 'chronological') { | ||
| // ChronologicalGallery exists in your repo; pass-through props | ||
| return ( | ||
| <ChronologicalGallery | ||
| images={images as any} // ChronologicalGallery expects your original Image type | ||
| showTitle={true} | ||
| title={title} | ||
| onMonthOffsetsChange={onMonthOffsetsChange} | ||
| scrollContainerRef={scrollableRef} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary type cast.
The images as any cast at line 77 bypasses TypeScript's type checking. Since AnyImage is already designed to be permissive and compatible with MediaImage, this cast should be unnecessary.
Apply this diff:
<ChronologicalGallery
- images={images as any} // ChronologicalGallery expects your original Image type
+ images={images}
showTitle={true}
title={title}
onMonthOffsetsChange={onMonthOffsetsChange}
scrollContainerRef={scrollableRef}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (mode === 'chronological') { | |
| // ChronologicalGallery exists in your repo; pass-through props | |
| return ( | |
| <ChronologicalGallery | |
| images={images as any} // ChronologicalGallery expects your original Image type | |
| showTitle={true} | |
| title={title} | |
| onMonthOffsetsChange={onMonthOffsetsChange} | |
| scrollContainerRef={scrollableRef} | |
| /> | |
| ); | |
| } | |
| if (mode === 'chronological') { | |
| // ChronologicalGallery exists in your repo; pass-through props | |
| return ( | |
| <ChronologicalGallery | |
| images={images} | |
| showTitle={true} | |
| title={title} | |
| onMonthOffsetsChange={onMonthOffsetsChange} | |
| scrollContainerRef={scrollableRef} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/GalleryView.tsx around lines 73 to 84, remove the
unnecessary type cast "images as any" when passing images to
ChronologicalGallery; instead pass images directly (images) so TypeScript
preserves proper typing since AnyImage is compatible with MediaImage, and update
any imports/types if the prop type mismatch resurfaces to ensure
ChronologicalGallery accepts the existing images type.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/pages/Home/Home.tsx (1)
66-71: Remove the type cast on images (duplicate concern).The
images as anytype cast on line 67 bypasses type safety. As noted in the previous review, the AnyImage type in GalleryView is permissive enough to accept the Image type without this cast.Apply this diff to remove the unnecessary type cast:
- <GalleryView - images={images as any} + <GalleryView + images={images} title={title} scrollableRef={scrollableRef} onMonthOffsetsChange={setMonthMarkers} />
🧹 Nitpick comments (1)
frontend/src/pages/Home/Home.tsx (1)
59-59: Consider conditionally rendering the ViewModeToggle.The ViewModeToggle is currently visible even when the gallery is empty. For better UX, consider only showing it when images are available.
Apply this diff to conditionally render the toggle:
- <ViewModeToggle /> + {images.length > 0 && <ViewModeToggle />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/Home/Home.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/pages/Home/Home.tsx (3)
frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)frontend/src/components/ViewModeToggle.tsx (1)
ViewModeToggle(82-116)frontend/src/components/GalleryView.tsx (1)
GalleryView(64-181)
🔇 Additional comments (1)
frontend/src/pages/Home/Home.tsx (1)
17-18: LGTM!The new imports for ViewModeToggle and GalleryView follow the project's import conventions and are properly structured.
|
|
|
hello @rahulharpal1603 sir |
|
Hello @rahulharpal1603 sir Check If my work is good till now so Approve and merge this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/GalleryView.tsx (1)
86-97: Remove unnecessary type cast.The
images as anycast at line 90 bypasses TypeScript's type checking. SinceAnyImageis already designed to be permissive and extendsPartial<MediaImage>, this cast should be unnecessary.Apply this diff:
<ChronologicalGallery - images={images as any} // ChronologicalGallery expects your original Image type + images={images} showTitle={true} title={title} onMonthOffsetsChange={onMonthOffsetsChange} scrollContainerRef={scrollableRef} />
🧹 Nitpick comments (3)
frontend/src/components/GalleryView.tsx (3)
120-120: Remove unnecessary type cast for alt text.The type cast
as stringis unnecessary because the expression(img.name || img.filename || String(img.id))already guarantees a string result through theString()fallback.Apply this diff:
- alt={(img.name || img.filename || String(img.id)) as string} + alt={img.name || img.filename || String(img.id)}Also applies to lines 156, 206.
185-185: Minor: Inconsistent spacing before comment.Extra whitespace before the comment at line 185 is inconsistent with the rest of the file (compare with lines 99, 134).
Apply this diff:
- - // MASONRY + // MASONRY
111-116: Consider extracting the keyboard handler to reduce duplication.The
onKeyDownhandler is duplicated across grid, list, and masonry modes. Extracting it into a reusable function would improve maintainability.For example, add this helper near
handleImageClick:const handleKeyDown = useCallback( (index: number, e: React.KeyboardEvent) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); handleImageClick(index); } }, [handleImageClick], );Then use it in each mode:
- onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - handleImageClick(index); - } - }} + onKeyDown={(e) => handleKeyDown(index, e)}Also applies to lines 146-151, 196-201.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/GalleryView.tsx(1 hunks)frontend/src/pages/Home/Home.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/GalleryView.tsx (5)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(10-14)ChronologicalGallery(25-187)frontend/src/app/store.ts (1)
RootState(26-26)frontend/src/features/imageSelectors.ts (1)
selectIsImageViewOpen(13-16)frontend/src/features/imageSlice.ts (1)
setCurrentViewIndex(22-34)frontend/src/components/Media/MediaView.tsx (1)
MediaView(25-224)
🔇 Additional comments (2)
frontend/src/components/GalleryView.tsx (2)
1-37: LGTM: Clean type definitions and imports.The imports are appropriate, and the
AnyImagetype is well-designed to accommodate different image shapes (offline/Tauri vs. online). The JSDoc comment clearly explains the tolerance for different image structures.
39-66: LGTM: Robust helper functions with proper error handling.Both
toTauriUrlandgetImageSrcimplement sensible fallback chains and handle edge cases gracefully. The path normalization and error handling ensure compatibility across Tauri and non-Tauri environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/GalleryView.tsx (2)
155-167: Removeimages as anywhen passing toChronologicalGallery.Casting
imagestoanydisables type checking betweenAnyImageand the image type expected byChronologicalGallery. GivenAnyImageis already designed to be compatible with your existing image shape, it’s better to keep type safety and adjust types if they don’t line up.If
ChronologicalGalleryexpectsMediaImage[], you can either:
- Update its props to accept the more general
AnyImage[], or- Narrow
imagesearlier (e.g., ensure they’reMediaImage[]) and pass them withoutany:- <ChronologicalGallery - images={images as any} // ChronologicalGallery expects your original Image type + <ChronologicalGallery + images={images} showTitle={true} title={title} onMonthOffsetsChange={onMonthOffsetsChange} scrollContainerRef={scrollableRef} />This keeps the compiler helping you instead of hiding potential mismatches.
203-205: RestoreMediaView’sonClosehandler and avoid repeated unsafe casts.For grid, list, and masonry modes you currently render:
{isImageViewOpen && <MediaView images={images as MediaImage[]} />}Issues:
- Missing
onClosehandler – Previous reviews notedMediaViewexpects anonCloseprop so users can exit the viewer. Without it, the viewer may be non‑dismissable, which is a critical UX bug.- Repeated
images as MediaImage[]casts – These bypass type checking and assume everyAnyImagemeetsMediaImage’s requirements. If they diverge, you’ll only find out at runtime.- Duplicate logic – The same
MediaViewsnippet is repeated in three branches and can easily get out of sync.Consider:
- {isImageViewOpen && <MediaView images={images as MediaImage[]} />} + {isImageViewOpen && ( + <MediaView + images={images as MediaImage[]} + onClose={() => dispatch(setCurrentViewIndex(-1))} + /> + )}and applying the same change in the list and masonry branches. Longer‑term, you could extract a small helper (e.g.,
renderMediaView()) to avoid copy‑pasting this block and to centralize any future prop changes.Also applies to: 255-257, 303-304
🧹 Nitpick comments (3)
frontend/src/components/ViewModeToggle.tsx (1)
23-55: Consider adding explicit accessible labels (or visible text) to the toggle buttons.Right now each button is icon‑only and relies on
title={m.label}as its label. Title is not a reliable accessible name across assistive tech and is invisible on touch devices. Consider either:
- Adding
aria-label={m.label}to the<button>, and/or- Rendering a visually hidden
<span className="sr-only">{m.label}</span>inside the button (or even visible text on larger screens).This keeps the UI clean while making the control more discoverable and screen‑reader friendly.
frontend/src/components/GalleryView.tsx (2)
35-40: Tighten Props typing: avoidanyunion and redundant| nullinRefObject.
onMonthOffsetsChange: (v: MonthMarker[] | any) => void;effectively collapses toany. IfChronologicalGalleryreally emitsMonthMarker[], prefer a precise type like(v: MonthMarker[]) => void(orMonthMarker[] | undefinedif needed).scrollableRef: React.RefObject<HTMLDivElement | null>;is redundant becauseRefObject<T>already exposescurrent: T | null. UseReact.RefObject<HTMLDivElement>instead for clearer typing.These tweaks keep the API self‑documenting and preserve TypeScript’s checking.
89-96: Avoid skipping valid ids when toggling favourites.
handleToggleFavouriteusesif (img?.id), which will skip ids like0or an empty string even though they’re valid identifiers elsewhere (you already useString(img.id)for React keys).Prefer a null/undefined check instead, for example:
- if (img?.id) { - toggleFavourite(String(img.id)); - } + if (img && img.id != null) { + toggleFavourite(String(img.id)); + }This ensures all legitimate ids participate in favourites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/GalleryView.tsx(1 hunks)frontend/src/components/ViewModeToggle.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ViewModeToggle.tsx (2)
frontend/src/features/viewModeSlice.ts (1)
ViewMode(4-4)frontend/src/app/store.ts (1)
RootState(26-26)
|
Hello @rahulharpal1603 sir . Please give response to My PR . |
Add View Mode Toggle for Gallery
Issue
Fixes #633
Fixes #633
Screenshots
Summary by CodeRabbit
New Features
Quality
Chores
✏️ Tip: You can customize this high-level summary in your review settings.